-
-
Notifications
You must be signed in to change notification settings - Fork 304
fix(1694): changelog_merge_prerelease not working on cz bump #1700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v4-11-0
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4-11-0 #1700 +/- ##
==========================================
Coverage ? 97.87%
==========================================
Files ? 60
Lines ? 2639
Branches ? 0
==========================================
Hits ? 2583
Misses ? 56
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
woile
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, thanks!
commitizen/commands/bump.py
Outdated
| "incremental": True, | ||
| "dry_run": dry_run, | ||
| "during_version_bump": self.arguments["prerelease"] | ||
| is None, # We let the changelog implementation know that we want to replace prereleases while staying incremental AND the new tag does not exist already |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is a bit unclear to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair. Was more of a note to self. I changed it to something more meaningful.
commitizen/changelog_formats/base.py
Outdated
| latest_version_index = index | ||
| line = line.strip().lower() | ||
|
|
||
| parsed = self.parse_version_from_title(line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rename the variable parsed to something more clear? Such as parsed_version. As well as the parsed in another function get_metadata_from_file.
Thanks!
commitizen/changelog_formats/base.py
Outdated
| if parsed: | ||
| if not self.tag_rules.extract_version( | ||
| GitTag(parsed.tag, "", "") | ||
| ).is_prerelease: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine these if statements with and
|
@woile @bearomorphism Thank you for your reviews! |
|
The latest commit should not be chore. I'd suggest you to rebase the commits and adjust the commit messages, so the commit history will be cleaner after merging |
|
I believe there is a way to mock the datetime in your new tests |
|
I remember git commit timestamp can be specified Thanks!🙏 |
646cc6c to
a16ff63
Compare
|
@bearomorphism Thanks again for the review!
I used freezegun to set a fixed time.
I tried that, but it shows a large diff now. Locally, if I do |
|
@zimmermannJakob could you try to rebase to v4-11-0? Thanks. |
a16ff63 to
9cb370d
Compare
9cb370d to
e9f5ba9
Compare
|
@bearomorphism Thank you, the diff looks good now! Anyway, the PR is good to go from my side. |
|
|
||
| - output glitch | ||
|
|
||
| ## 0.1.0 (1970-01-01) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question, is this expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you mean the empty version 0.1.0?
I would say yes. Reason is that an annotated tag is created on a version that has no eligible commits e.g. git commit -m "irrelevant commit" (see testcase). The Changelog command renders the release in that case as empty.
If this is not intended, I can modify the testcase to make it look 'more pretty' and open a new issue describing this bug. But I think it works as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for detailed explanation. I don't think it's a blocker.
@woile wdyt
I would say it depends. I think PR creators can close the discussion themselves once they think the issue is resolved, but I also have seen in some cultures it's the reviewers closing the discussion. Another option is to simply reply "fixed", but there are also people think it looks verbose if under every discussion there is a "fixed" comment. |
Description
This PR solves issue #1694.
Checklist
Code Changes
poetry alllocally to ensure this change passes linter check and tests (Some error out on mac. I will investigate if they fail in ci)- [ ] Update the documentation for the changes(I do not think this applies. It works now as documented.)### Documentation Changes- [ ] Runpoetry doclocally to ensure the documentation pages renders correctly- [ ] Check and fix any broken links (internal or external) in the documentationExpected Behavior
See issue #1694.
Steps to Test This Pull Request
poetry build.Additional Context
Known issue: When the changelog only contains pre-releases, the old behavior can still be observed.(Fixed)